-
Notifications
You must be signed in to change notification settings - Fork 2.6k
[CI] Improve on metrics comment #7449
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CI] Improve on metrics comment #7449
Conversation
Signed-off-by: pipiland2612 <[email protected]>
Signed-off-by: pipiland2612 <[email protected]>
Signed-off-by: pipiland2612 <[email protected]>
This is so strange, my change is not even related to e2e test for some functionality, but all the e2e test keeps failing |
Metrics Comparison Summarysummary_metrics_snapshot_kafka📊 Metrics Diff SummaryTotal Changes: 3
🔄 Modified Metrics
summary_metrics_snapshot_cassandra📊 Metrics Diff SummaryTotal Changes: 53
🆕 Added Metrics
summary_metrics_snapshot_elasticsearch📊 Metrics Diff SummaryTotal Changes: 73
❌ Removed Metrics
summary_metrics_snapshot_cassandra📊 Metrics Diff SummaryTotal Changes: 53
🆕 Added Metrics
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7449 +/- ##
===========================================
+ Coverage 33.68% 96.63% +62.95%
===========================================
Files 202 377 +175
Lines 12986 23089 +10103
===========================================
+ Hits 4374 22312 +17938
+ Misses 8269 592 -7677
+ Partials 343 185 -158
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Hi @yurishkuro, I don't know why some metrics are changed(may be e2e test is non deterministic at some parts). But this pr should be good now. |
yes we need to fix this. In Kafka e2e tests we use randomized topic names and the topic is added as a label to the metrics, causing the differences. We may need to add some sanitization to the metric dump steps to remove random suffixes in metric labels. Don't know about cassandra, it's possible it's the same reason, a randomized namespace inserted as labels. Btw it would be useful to include a sample of diffs, e.g. 5-7 lines, in the report. We can expect the number of diffs to be relatively small across PRs, and it's much easier to see the report directly than to download and unpack the artifacts. |
Hi @yurishkuro, |
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger
:make lint test
jaeger-ui
:npm run lint
andnpm run test